Eager safety net: flush when the task ring is full, not only the payload - #155
claude[bot] wants to merge 1 commit into
Conversation
A legacy step with more firing hooks than task_ring_entries gets STEP_OVERSIZED and runs every hook through the eager safety net in HookPoint.forward. The net checked only payload bytes before reserve_one, and reserve_one advanced the task head with no task-capacity check, so hook task_cap reserved a sequence whose producer publishes into slot 0 while hook 0's READY word is still unread. The drain then pairs the wrong size with a TensorMeta, or clears the new word and waits at that sequence forever while flush_and_wait reports success. Found by TLA+ model checking of the payload ring. - RingEnginePy::available_task_slots() (bound to Python) reports free task entries, computed like available_capacity(). - The safety net reserves without a flush only when bytes AND a task entry are free; otherwise it takes the flush-then-reserve branch. - reserve_one throws std::logic_error, reserving nothing, when no task entry is free, so no caller can overwrite an unread slot silently. - estimate.py no longer claims the over-task-cap case goes CPU-direct.
XbzOnGit
left a comment
There was a problem hiding this comment.
Reviewed revision 6c48a66. No blocking correctness issues found.
The eager path checks both aligned payload capacity and a free task slot before reserving, and the native reserve_one guard refuses overflow before advancing either head. The check-and-reserve sequence is sound under the existing single-producer contract.
Built and tested this exact revision in a separate worktree using the fresh Conda environment and an RTX 4090:
- 43 focused Python/GPU tests passed, including independent regressions with 1, 2, and 4 task slots, repeated wraparound, zero-byte eager routing, full payload reclamation, and record-mode refusal.
- 34 CPU adapter/planning tests passed.
- All six native test executables passed: 633 checks.
- An additional native delivery regression passed 27 checks, including byte-for-byte payload and metadata ordering for nine hooks through a two-slot ring.
Nonblocking recommendation: retain the real-GPU wraparound/delivery regression in the repository. The additional regressions above were run locally against this revision and are not currently included in this PR.
Approved.
|
@XbzOnGit thanks for running it on a GPU. Agreed on keeping the wraparound/delivery regression — I don't have that test here, so if you push it to this branch (or paste it) I'll wire it into the native test target and keep CI green. Generated by Claude Code |
6c48a66 to
3763122
Compare
…oad (#160) * Eager safety net: flush when the task ring is full, not only the payload * Keep a real-GPU delivery regression for the eager task-ring flush * Blame bytes, not task entries, when only the bytes overflow * Read a legacy ring past its capacity as full, not as ~2^64 free * Name a failed drain when reserve_one finds no free task entry * Drive the eager safety net through HookPoint and pybind on a real ring * Reserve a needs_eager step once: per hook, not also as a whole Replaces #155.
Requested by Alan Liu · Slack thread
Before: In the legacy ring, a step that fires more hooks than the task ring has entries could quietly lose or mislabel captured tensors. The eager fallback only checked that there was enough payload space. It never checked for a free task slot, so the hook after the ring filled wrote over a slot the drain had not read yet.
flush_and_waitstill reported success.After: The eager fallback flushes the ring when either payload space or task slots run out, so every hook gets a free slot.
reserve_oneitself now refuses (raises) instead of overwriting when no task slot is free. Record-mode rings were never affected and behave the same.How. Found by TLA+ model checking of the payload ring (
CapacityRespectedand thenNoSlotOverwritefail in 12–15 steps). The failure path, confirmed againstmain @ a987dfe:prepare_stepseesnum_hooks > task_cap, flushes, and returnsSTEP_OVERSIZED. The adapter setsforce_eager.HookPoint.forwardchecks onlyavailable_capacity()(payload bytes) and callsreserve_one. That callsDrainThread::reserve(n, 1), which advances the task head with no task-capacity check.task_caprelease-stores itsREADY|sizeword into slot 0 while hook 0's word is still unread (producers never read the tails). The drain then either reads the wrong size, shifting the TensorMeta pairing by one, or clears the new word inflush_state_updateand waits at that sequence forever.The fix adds
RingEnginePy::available_task_slots(), computed and bound the same way asavailable_capacity(). The safety net takes the no-flush branch only when the bytes fit and at least one task entry is free. Otherwise it flushes and then reserves, and a flush frees both. As a second guard,reserve_onethrowsstd::logic_errorand reserves nothing when no task entry is free. The ring's only otherreserve_onecaller is this safety net, and it now checks first, so no correct path hits the throw. Theestimate.pymessage that said the over-task-cap case "falls back to eager CPU-direct dispatch" is corrected, and the check-and-reserve comments inring_engine_py.cunow cover task slots.Tests
tests/test_hook_point_eager_task_slots.py: a fake legacy ring with 4 task entries and plenty of payload fires 5 hooks through the eager safety net. It asserts that no more than 4 tasks are ever outstanding and that the 5th hook flushes before it reserves. Fails onmain(assert 5 <= 4), passes with the fix.test_reserve_one_refuses_without_a_free_task_slotintests/native/ring/test_ring_engine.cu.test_hook_point_eager_cap_cache.pyandtest_producer_chunked_schema.pygainavailable_task_slots().python -m pytest -m cpu -q: 1944 passed, 324 skipped (the skips are native binaries not built here).ruff check --select F821 src/dmiis clean.bindings.cpppassedg++ -fsyntax-onlyagainst the torch/pybind headers. A host-sideg++parse ofring_engine_py.cuandtest_ring_engine.cushows the same errors asmain(only__CUDACC__-gated launcher declarations) and none in the touched code. CI's native compile is the real build check.